Fix Exp/Power fit docs to match returned log-space intercept (closes #110)#111
Closed
ChrisRackauckas-Claude wants to merge 1 commit into
Closed
Conversation
`ExpCurveFitAlgorithm` and `PowerCurveFitAlgorithm` linearize in log space, so the returned `sol.u = (a, b)` has `b` equal to the intercept in log space, not the multiplicative scale factor. The docstrings and README claimed the model was `y = b*exp(a*x)` / `y = b*x^a` with `b` returned directly, which is inconsistent with the actual `(a, log(b))` output (as already demonstrated by the tutorial's `exp(sol.u[2])` recovery and the `sol.u[2] ≈ log(2.0)` assertions in test/linfit_exp.jl and test/linfit_power.jl). Restate the models as `y = exp(a*x + b)` and `y = exp(a*log(x) + b)`, matching the linear fit `log(y) = a*f(x) + b`, and document that the multiplicative scale factor is recovered as `exp(b)`. Closes SciML#110 Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5bhhA1XYyVaBq2YQfKiRz
Member
|
I don't think this is a good fix, it seems more user friendly to return the scale factor. I have another branch in progress that also fixes some other things, will put it up in a bit. |
Member
|
We should at least document it correctly. But if you have a fix to make it match the docs in code, that would probably be better yeah... |
5 tasks
Member
|
I'll close this since #112 is in now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #110.
ExpCurveFitAlgorithmandPowerCurveFitAlgorithmlinearize in log space viaLinearCurveFitAlgorithm, which fitslog(y) = a*f(x) + band returnssol.u = (a, b). Herebis the intercept in log space — i.e.logof the multiplicative scale factor — not the scale factor itself.The docstrings and README, however, advertised the models as
y = b*exp(a*x)andy = b*x^awithbreturned directly. That is inconsistent with the actual(a, log(b))output, as reported in the issue:The current behavior is intentional and already reflected elsewhere in the codebase:
docs/src/tutorials/getting_started.mdrecovers the scale factor withexp(sol.u[2]).test/linfit_exp.jlandtest/linfit_power.jlassertsol.u[2] ≈ log(2.0).This PR fixes only the documentation to match that behavior (the non-breaking resolution requested in the issue's "Expected behavior"):
ExpCurveFitAlgorithm,PowerCurveFitAlgorithm): restate the models asy = exp(a*x + b)andy = exp(a*log(x) + b), matching the linear fitlog(y) = a*f(x) + b, and note that the scale factor of the equivalenty = B*exp(a*x)/y = B*x^aform is recovered asB = exp(b).Why docs and not code
The returned-parameter semantics are part of the v1 public API and are already pinned by the existing test assertions (
sol.u[2] ≈ log(2.0)) and the tutorial'sexp(sol.u[2]). Changing the returned values to the multiplicative form would be a breaking change and would also complicate the genericLinearCurveFitAlgorithmabstraction (which returns the raw log-space intercept for anyyfun). The issue's stated expected behavior is the documentation fix.Testing
The behavior is already covered by
test/linfit_exp.jlandtest/linfit_power.jl, both of which assertsol.u[2] ≈ log(2.0). Ran both locally — 7/7 pass each. Runic reports no changes needed onsrc/common_interface.jl.This PR should be ignored until reviewed by @ChrisRackauckas.
🤖 Generated with Claude Code